Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix several panics in ParseCalendar #85

Merged
merged 4 commits into from
Jan 28, 2024
Merged

Conversation

brackendawson
Copy link
Contributor

I was trying to Fuzz webcal-proxy but I need golang-ical to be panic free first.

@arran4
Copy link
Owner

arran4 commented Jan 27, 2024

Thanks. Looks good. Running tests.

calendar_test.go Outdated
f.Add(ics)
f.Fuzz(func(t *testing.T, ics []byte) {
_, err := ParseCalendar(bytes.NewReader(ics))
t.Log(err)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't this be t.Errorf?

Copy link
Contributor Author

@brackendawson brackendawson Jan 28, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, returning an error is fine, most of the corpus data will be invalid ics. Only a panic is a failure here. I added the log line because I wanted to see the errors I wrote using go test -v.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Acknowledged. I haven't used the fuzzing feature myself. Not nearly enough go coding these days.

arran4
arran4 previously approved these changes Jan 27, 2024
@arran4
Copy link
Owner

arran4 commented Jan 27, 2024

Tests are failing for pre-fuzzing go. Perhaps use build tags?

Something like:

// +build go1.18

But that is not correct as it isn't forwards compatible I think. I rarely use go version build constraints

@arran4
Copy link
Owner

arran4 commented Jan 27, 2024

Hrm.. Should modify the github workflow action to test on more recent versions of go as well.

@brackendawson
Copy link
Contributor Author

Hrm.. Should modify the github workflow action to test on more recent versions of go as well.

I notice you test from 1.14 but the go directive in the mod file is 1.13.

@arran4
Copy link
Owner

arran4 commented Jan 28, 2024

Cool all tests pass.

I notice you test from 1.14 but the go directive in the mod file is 1.13.

Interesting. I didn't notice. That would have happened by accident over time rather than intentional. I don't see a reason to change it right now but will accept a PR/change that pushes the matrix back.

Copy link
Owner

@arran4 arran4 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@arran4 arran4 merged commit 51fa6f1 into arran4:master Jan 28, 2024
13 checks passed
@arran4
Copy link
Owner

arran4 commented Jan 28, 2024

% git-tag-inc release && git push --tags
Largest: v0.2.3 (51fa6f12130177fd8e6ef8e6e5fcc79e28141324)
Creating v0.2.4
warning: not sending a push certificate since the receiving end does not support --signed push
Enumerating objects: 1, done.
Counting objects: 100% (1/1), done.
Writing objects: 100% (1/1), 155 bytes | 155.00 KiB/s, done.
Total 1 (delta 0), reused 0 (delta 0), pack-reused 0
To https://github.com/arran4/golang-ical.git
 * [new tag]         v0.2.4 -> v0.2.4

Tagged to release for you

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants